Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Query interface #1086

Merged
merged 1 commit into from
Aug 30, 2019
Merged

Query interface #1086

merged 1 commit into from
Aug 30, 2019

Conversation

00vareladavid
Copy link
Contributor

@00vareladavid 00vareladavid commented Feb 25, 2019 •
edited
Loading

This provides a low-level interface that handles queries about the dependency graph.

@00vareladavid 00vareladavid changed the title Query interface [RFC] Query interface Feb 25, 2019
@00vareladavid 00vareladavid force-pushed the 00/add/info branch 2 times, most recently from c9e4512 to 13a84c0 Compare February 25, 2019 07:37
@00vareladavid
Copy link
Contributor Author

00vareladavid commented Feb 25, 2019 •
edited
Loading

Two parts:

  • choose a subset of the graph
    • active, direct, and all (could probably use better names)
  • choose properties to query
    • currently supports [name pinned rev dev source stdlib version url]

@KristofferC
Copy link
Member

An official interface for asking these type of queries (that we keep stable) would greatly help people working on stuff like the VSCode plugin I think.

@fredrikekre
Copy link
Member

I had also planned to do something similar to this. I was thinking of just creating an interface to return the TOML-parsed Dict of Projetc.toml and then you can do whatever you want with that. Not sure what is better, or if they are orthogonal to each other.

@00vareladavid 00vareladavid mentioned this pull request Feb 25, 2019
Closed
@00vareladavid
Copy link
Contributor Author

00vareladavid commented Feb 25, 2019 •
edited
Loading

Perhaps orthogonal, this is intended for other packages to more easily hook into Pkg. It might be too heavy for people who e.g. want to query the version of their own project.

Its possible, but cumbersome as a human interface: collect(values(Pkg.info(;active=true, version=true)))[1]["version"].

A human based interface might just support active project + direct deps so that the result can be indexed by name, as opposed to UUID.

@KristofferC
Copy link
Member

Putting this as a reference https://doc.rust-lang.org/cargo/reference/external-tools.html#information-about-package-structure.

@KristofferC
Copy link
Member

One idea which might be a bit more future proof is to make a dedicated struct that holds the information instead of a Dict{String, Any}. That was we can deprecate things and tab completion will work etc.

@fredrikekre fredrikekre mentioned this pull request Mar 30, 2019
Closed
@davidanthoff
Copy link

So in general I would love to see a stable API. In practice, for VS Code, the issue is that we want to support julia 1.0 going forward, given that it is the long term release. So anything that just appears in say in julia 1.2 or julia 1.3 doesn't help us much... In fact, what really would help us would be a package (not in the stdlib) that hides all the differences between different julia 1.x versions for us.

@KristofferC
Copy link
Member

In fact, what really would help us would be a package (not in the stdlib) that hides all the differences between different julia 1.x versions for us.

I don't think anyone here really has plan on doing that. Using the "query interface" whenever it gets in a release and "ifdef" for earlier versions would likely be your best option.

@00vareladavid 00vareladavid changed the title [RFC] Query interface Query interface May 30, 2019
@00vareladavid
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request May 31, 2019
@bors
Try #1086:
7c79ea8
@bors
Copy link
Contributor

bors bot commented May 31, 2019

try

Build failed

@00vareladavid 00vareladavid force-pushed the 00/add/info branch 3 times, most recently from 49f6575 to 885594a Compare May 31, 2019 17:15
@00vareladavid
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request May 31, 2019
@bors
Try #1086:
ae77186
@bors
Copy link
Contributor

bors bot commented May 31, 2019

try

Build succeeded

@00vareladavid
Copy link
Contributor Author

I feel this is good to merge. Any objections/feedback?

As Kristoffer suggested, the data is returned as a struct instead of a dictionary. I also added documentation and some basic tests. I also remove the old __installed API which serves a similar purpose but is based on names, not UUIDs.

Since this is supposed to be a bit more future proof. I went with the conservative approach of requiring users to explicitly specify all the properties that they want to query. The idea is that future properties might be more a lot more expensive to compute (I'm thinking of cases where we have to parse registry info). Feedback on this approach specifically?

I was also conservative with the initial set of properties that users can query. We would definitely add more properties as we understand the kinds of questions that users ask.

oxinabox
oxinabox reviewed Jun 4, 2019
View reviewed changes
src/Pkg.jl
| `developed` | `Bool` | Whether a package is directly tracking a directory |
| `pinned` | `Bool` | Whether a package is pinned |
| `source` | `String` | The directory containing the package's source code |

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which of these will be nothing for stdlibs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All fields should "Just Work" for stdlibs, with the exception of version which will always be nothing.

oxinabox
oxinabox reviewed Jun 4, 2019
View reviewed changes
src/Pkg.jl Show resolved Hide resolved
@00vareladavid 00vareladavid force-pushed the 00/add/info branch 2 times, most recently from fb7998a to f0cd887 Compare August 22, 2019 04:37
@00vareladavid

This comment has been minimized.

Sign in to view
bors bot added a commit that referenced this pull request Aug 22, 2019
@bors
Try #1086:
09a2b40
@bors

This comment has been minimized.

Sign in to view
@00vareladavid
Copy link
Contributor Author

I'm fairly happy with this. Unless anyone still has strong opinions, I will rebase and merge

KristofferC
KristofferC reviewed Aug 26, 2019
View reviewed changes
src/Pkg.jl
@@ -273,6 +270,41 @@ const develop = API.develop
#TODO: Will probably be deprecated for something in PkgDev
const generate = API.generate

"""
Pkg.dependencies()::Dict{UUID, PackageInfo}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this include things only in the Project, or also in the Manifest or should that be an option?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you mention it, perhaps people might want to use for (uuid, pkg) in Pkg.dependencies()? In which case a filter keyword does seem useful.

KristofferC
KristofferC reviewed Aug 26, 2019
View reviewed changes
src/Pkg.jl
const dependencies = API.dependencies

"""
Pkg.project()::ProjectInfo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if ProjectInfo(::UUID) would be a good constructor? Otherwise it is a bit hard to do something with the .dependencies field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean. What would be the result of ProjectInfo(::UUID)? What UUID would you give it?

@00vareladavid 00vareladavid force-pushed the 00/add/info branch 2 times, most recently from 27504e1 to 68f650f Compare August 30, 2019 01:27
@00vareladavid

This comment has been minimized.

Sign in to view
bors bot added a commit that referenced this pull request Aug 30, 2019
@bors @00vareladavid
Merge #1086
b81aa52 
1086: Query interface r=00vareladavid a=00vareladavid

This provides a low-level interface that handles queries about the dependency graph.

Co-authored-by: David Varela <[email protected]>
@bors

This comment has been minimized.

Sign in to view
@00vareladavid

This comment has been minimized.

Sign in to view
bors bot added a commit that referenced this pull request Aug 30, 2019
@bors @00vareladavid
Merge #1086
600b0fc 
1086: Query interface r=00vareladavid a=00vareladavid

This provides a low-level interface that handles queries about the dependency graph.

Co-authored-by: David Varela <[email protected]>
@bors

This comment has been minimized.

Sign in to view
@00vareladavid
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Aug 30, 2019
@bors @00vareladavid
Merge #1086
95ae6d7 
1086: Query interface r=00vareladavid a=00vareladavid

This provides a low-level interface that handles queries about the dependency graph.

Co-authored-by: David Varela <[email protected]>
@bors
Copy link
Contributor

bors bot commented Aug 30, 2019

Build succeeded

@bors bors bot merged commit 996c6b9 into JuliaLang:master Aug 30, 2019
@00vareladavid 00vareladavid mentioned this pull request Sep 7, 2019
Closed
@giordano giordano mentioned this pull request Sep 28, 2019
Merged
@NHDaly NHDaly mentioned this pull request Oct 3, 2019
Closed
@fjarri fjarri mentioned this pull request Oct 17, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxinabox oxinabox oxinabox approved these changes

@KristofferC KristofferC KristofferC approved these changes

@fredrikekre fredrikekre Awaiting requested review from fredrikekre

Assignees
No one assigned
Labels
API feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@00vareladavid @KristofferC @fredrikekre @davidanthoff @oxinabox @timholy @StefanKarpinski